kvserver: use EncodedError in SnapshotResponse #75248

tbg · 2022-01-20T21:42:30Z

We were previously using a "Message" string to indicate details about an
error. We can do so much better now and actually encode the error. This
wasn't possible when this field was first added, but it is now, so let's
use it. As always, there's a migration concern, which means the old
field stays around & is populated as well as interpreted for one
release.

We then use this new-found freedom to improve which errors were marked
as "failed snapshot" errors. Previously, any error coming in on a
SnapshotResponse were considered snapshot errors and were considered
retriable. This was causing TestLearnerSnapshotFailsRollback to run
for 90s, as TestCluster's replication changes use a SucceedsSoon to
retry snapshot errors - but that test actually injects an error that it
wants to fail-fast. Now, since snapshot error marks propagate over the
wire, we can do the marking on the sender of the SnapshotResponse, and
we can only mark messages that correspond to an actual failure to apply
the snapshot (as opposed to an injected error, or a hard error due to a
malformed request). The test now takes around one second, for a rare 90x
speed-up.

As a drive-by, we're also removing errMalformedSnapshot, which became
unused when we stopped sending the raft log in raft snaps a few releases
back, and which had managed to hide from the unused lint.

Fixes #74621.
Fixes #87337.

Release note: None

cockroach-teamcity · 2022-01-20T21:42:37Z

This change is

lidorcarmel

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

lidorcarmel

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

pkg/kv/kvserver/replica_command.go, line 2512 at r2 (raw file):

		return err
	}
	return nil

maybe just:

	return r.store.cfg.Transport.SendSnapshot(
		ctx,
		r.store.allocator.storePool,
		req,
		snap,
		newBatchFn,
		sent,
	)

instead of everything? or maybe you like the existing pattern :)

Suggestion:

	if err := r.store.cfg.Transport.SendSnapshot(
		ctx,
		r.store.allocator.storePool,
		req,
		snap,
		newBatchFn,
		sent,
	); err != nil {
		return err
	}
	return nil

tbg

TFTR, Lidor!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lidorcarmel)

pkg/kv/kvserver/replica_command.go, line 2512 at r2 (raw file):

Previously, lidorcarmel (Lidor Carmel) wrote…

maybe just:
	return r.store.cfg.Transport.SendSnapshot(
		ctx,
		r.store.allocator.storePool,
		req,
		snap,
		newBatchFn,
		sent,
	)
instead of everything? or maybe you like the existing pattern :)

Done.

lidorcarmel

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lidorcarmel)

These take >1m under race and are now skipped for the race build. Note that TestLearnerSnapshotFailsRollback takes 90s even under ideal conditions, though that is fixed once I get cockroachdb#75248 over the finish line. At that point it probably wouldn't figure prominently as a slow test, but until then it's verrrry slow (>7m, probably since race stuff slows down the longer it runs) so still good to skip it now. https://cockroachlabs.slack.com/archives/C0KB9Q03D/p1646221664032729 Release justification: testing-only change Release note: None

77272: kvserver: skip some slow tests under race r=erikgrinaker a=tbg These take >1m under race and are now skipped for the race build. Note that TestLearnerSnapshotFailsRollback takes 90s even under ideal conditions, though that is fixed once I get #75248 over the finish line. At that point it probably wouldn't figure prominently as a slow test, but until then it's verrrry slow (>7m, probably since race stuff slows down the longer it runs) so still good to skip it now. https://cockroachlabs.slack.com/archives/C0KB9Q03D/p1646221664032729 Release justification: testing-only change Release note: None Co-authored-by: Tobias Grieger <[email protected]>

aayushshah15

I'd noticed this recently while working on #77246 but couldn't see an easy way to work around it. TIL about errors.EncodedError.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lidorcarmel)

aayushshah15

@tbg should we merge this?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lidorcarmel)

tbg · 2022-03-28T08:19:36Z

I had put it off a bit to avoid changing master code in case we'd need some backport for the allocator. If you think this is fine to merge now I would go ahead and do so. I would certainly like to get it in to save 90s per test run :-)

aayushshah15 · 2022-03-29T16:03:13Z

It from that angle, so I'd be in favour of merging to master. This PR should speed up all the tests that induce failures to replication changes.

tbg · 2022-03-30T18:31:33Z

bors r=aayushshah15

craig · 2022-03-30T19:10:47Z

Build failed (retrying...):

GitHub CI (Cockroach)

tbg · 2022-03-30T19:13:29Z

bors r-

Looks like I need to shake out some flakes.

craig · 2022-03-30T19:13:30Z

Canceled.

tbg · 2022-09-28T20:42:59Z

Rebased. Tests seem to pass reliably (at least kvserver package), but this doesn't seem good:

--- PASS: TestReplicateQueueSwapVotersWithNonVoters (80.01s)

Going to take another look at slow tests before merging this. I bet lots of those aren't new:

$ less slow-tests.txt | grep -E '^... PASS: .*\([1-9][0-9]\.[0-9]+s\)$'
--- PASS: TestOptimisticEvalWithConcurrentWriters (10.54s)
--- PASS: TestLargeUnsplittableRangeReplicate (12.66s)
--- PASS: TestProtectedTimestamps (13.14s)
--- PASS: TestGetChecksumNotSuccessfulExitConditions (11.11s)
--- PASS: TestReplicateQueueShouldQueueNonVoter (15.74s)
--- PASS: TestReplicateQueueMetrics (23.49s)
--- PASS: TestSplitBurstWithSlowFollower (13.46s)
--- PASS: TestResetQuorum (42.88s)
--- PASS: TestReplicateQueueUpAndDownReplicateNonVoters (14.29s)
--- PASS: TestClosedTimestampCantServeForNonTransactionalBatch (19.95s)
--- PASS: TestClosedTimestampFrozenAfterSubsumption (19.99s)
--- PASS: TestReplicateRestartAfterTruncationWithRemoveAndReAdd (11.29s)
--- PASS: TestReplicateQueueDecommissioningNonVoters (28.87s)
--- PASS: TestNonVoterCatchesUpViaRaftSnapshotQueue (10.56s)
--- PASS: TestClosedTimestampCantServeWithConflictingIntent (10.27s)
--- PASS: TestClosedTimestampCanServeAfterSplitAndMerges (10.26s)
--- PASS: TestLearnerRaftConfState (12.83s)
--- PASS: TestSystemZoneConfigs (22.74s)
--- PASS: TestClosedTimestampCanServeThroughoutLeaseTransfer (10.95s)
--- PASS: TestReplicateQueueSwapVotersWithNonVoters (86.40s)
--- PASS: TestDecommission (14.99s)
--- PASS: TestBackpressureNotAppliedWhenReducingRangeSize (31.75s)
--- PASS: TestStoreRangeMergeTimestampCache (34.41s)
--- PASS: TestMergeQueueSeesNonVoters (21.09s)
--- PASS: TestProcessSplitAfterRightHandSideHasBeenRemoved (10.07s)
--- PASS: TestConsistencyQueueRecomputeStats (35.57s)
--- PASS: TestClosedTimestampCanServeForWritingTransaction (10.07s)
--- PASS: TestSnapshotsToDrainingNodes (59.17s)
--- PASS: TestClosedTimestampCantServeBasedOnUncertaintyLimit (11.15s)
--- PASS: TestLearnerSnapshotFailsRollback (92.51s)

tbg · 2022-09-28T20:53:15Z

Ironically TestLearnerSnapshotFailsRollback still takes 45s for each of the two cases 😆 well I'll go through these tests.

tbg

Dismissed @lidorcarmel from a discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @lidorcarmel)

tbg · 2022-10-31T14:58:39Z

PTAL @aayushshah15

=== RUN   TestLearnerSnapshotFailsRollback/learner
    --- PASS: TestLearnerSnapshotFailsRollback/learner (0.40s)
=== RUN   TestLearnerSnapshotFailsRollback/non-voter
    --- PASS: TestLearnerSnapshotFailsRollback/non-voter (0.39s)

aayushshah15

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @lidorcarmel)

We were previously using a "Message" string to indicate details about an error. We can do so much better now and actually encode the error. This wasn't possible when this field was first added, but it is now, so let's use it. As always, there's a migration concern, which means the old field stays around & is populated as well as interpreted for one release. We then use this new-found freedom to improve which errors were marked as "failed snapshot" errors. Previously, any error coming in on a `SnapshotResponse` were considered snapshot errors and were considered retriable. This was causing `TestLearnerSnapshotFailsRollback` to run for 90s, as `TestCluster`'s replication changes use a [SucceedsSoon] to retry snapshot errors - but that test actually injects an error that it wants to fail-fast. Now, since snapshot error marks propagate over the wire, we can do the marking on the *sender* of the SnapshotResponse, and we can only mark messages that correspond to an actual failure to apply the snapshot (as opposed to an injected error, or a hard error due to a malformed request). The test now takes around one second, for a rare 90x speed-up. As a drive-by, we're also removing `errMalformedSnapshot`, which became unused when we stopped sending the raft log in raft snaps a few releases back, and which had managed to hide from the `unused` lint. [SucceedsSoon]: https://github.com/cockroachdb/cockroach/blob/37175f77bf374d1bcb76bc39a65149788be06134/pkg/testutils/testcluster/testcluster.go#L628-L631 Fixes cockroachdb#74621. Release note: None

tbg · 2022-11-02T20:32:51Z

bors r=aayushshah15
TFTR!

craig · 2022-11-03T01:15:41Z

This PR was included in a batch that timed out, it will be automatically retried

craig · 2022-11-03T04:11:48Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-11-03T05:08:41Z

Build succeeded:

Bazel Essential CI (Cockroach)

tbg requested a review from a team as a code owner January 20, 2022 21:42

tbg requested a review from lidorcarmel January 20, 2022 21:42

lidorcarmel approved these changes Jan 20, 2022

View reviewed changes

tbg force-pushed the snaps-enc-err branch from 235f7d5 to 9a61822 Compare January 20, 2022 22:16

lidorcarmel approved these changes Jan 20, 2022

View reviewed changes

tbg requested a review from lidorcarmel January 21, 2022 08:18

tbg force-pushed the snaps-enc-err branch from 9a61822 to 040fab4 Compare January 21, 2022 08:18

tbg commented Jan 21, 2022

View reviewed changes

lidorcarmel approved these changes Jan 22, 2022

View reviewed changes

tbg mentioned this pull request Mar 2, 2022

kvserver: skip some slow tests under race #77272

Merged

tbg force-pushed the snaps-enc-err branch 2 times, most recently from e01eb26 to 8380321 Compare March 17, 2022 19:16

aayushshah15 approved these changes Mar 19, 2022

View reviewed changes

nvanbenschoten mentioned this pull request Mar 22, 2022

kvserver: slow tests #77951

Open

aayushshah15 reviewed Mar 25, 2022

View reviewed changes

tbg force-pushed the snaps-enc-err branch from 8380321 to 641c494 Compare March 30, 2022 12:54

aayushshah15 mentioned this pull request Sep 2, 2022

kv: TestSnapshotsToDrainingNodes spins on "retryable" snapshot error for 45s #87337

Closed

tbg force-pushed the snaps-enc-err branch 3 times, most recently from 8bf769d to 61d8135 Compare September 28, 2022 20:28

tbg force-pushed the snaps-enc-err branch 2 times, most recently from 09da176 to 8310d5b Compare October 31, 2022 14:55

tbg requested a review from lidorcarmel October 31, 2022 14:55

tbg commented Oct 31, 2022

View reviewed changes

tbg requested review from aayushshah15 and removed request for lidorcarmel October 31, 2022 16:20

tbg force-pushed the snaps-enc-err branch from 8310d5b to f67b49d Compare October 31, 2022 16:49

aayushshah15 approved these changes Nov 1, 2022

View reviewed changes

tbg force-pushed the snaps-enc-err branch from f67b49d to 4490ef1 Compare November 2, 2022 04:00

craig bot merged commit d2171af into cockroachdb:master Nov 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvserver: use EncodedError in SnapshotResponse #75248

kvserver: use EncodedError in SnapshotResponse #75248

tbg commented Jan 20, 2022 •

edited

Loading

cockroach-teamcity commented Jan 20, 2022

lidorcarmel left a comment

lidorcarmel left a comment

tbg left a comment

lidorcarmel left a comment

aayushshah15 left a comment

aayushshah15 left a comment

tbg commented Mar 28, 2022

aayushshah15 commented Mar 29, 2022

tbg commented Mar 30, 2022

craig bot commented Mar 30, 2022

tbg commented Mar 30, 2022

craig bot commented Mar 30, 2022

tbg commented Sep 28, 2022 •

edited

Loading

tbg commented Sep 28, 2022

tbg left a comment

tbg commented Oct 31, 2022

aayushshah15 left a comment

tbg commented Nov 2, 2022

craig bot commented Nov 3, 2022

craig bot commented Nov 3, 2022

craig bot commented Nov 3, 2022

kvserver: use EncodedError in SnapshotResponse #75248

kvserver: use EncodedError in SnapshotResponse #75248

Conversation

tbg commented Jan 20, 2022 • edited Loading

cockroach-teamcity commented Jan 20, 2022

lidorcarmel left a comment

Choose a reason for hiding this comment

lidorcarmel left a comment

Choose a reason for hiding this comment

tbg left a comment

Choose a reason for hiding this comment

lidorcarmel left a comment

Choose a reason for hiding this comment

aayushshah15 left a comment

Choose a reason for hiding this comment

aayushshah15 left a comment

Choose a reason for hiding this comment

tbg commented Mar 28, 2022

aayushshah15 commented Mar 29, 2022

tbg commented Mar 30, 2022

craig bot commented Mar 30, 2022

tbg commented Mar 30, 2022

craig bot commented Mar 30, 2022

tbg commented Sep 28, 2022 • edited Loading

tbg commented Sep 28, 2022

tbg left a comment

Choose a reason for hiding this comment

tbg commented Oct 31, 2022

aayushshah15 left a comment

Choose a reason for hiding this comment

tbg commented Nov 2, 2022

craig bot commented Nov 3, 2022

craig bot commented Nov 3, 2022

craig bot commented Nov 3, 2022

tbg commented Jan 20, 2022 •

edited

Loading

tbg commented Sep 28, 2022 •

edited

Loading